Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(http-plugin): add options to disable new spans if no parent #931 #948

Merged
merged 7 commits into from
May 6, 2020
Merged

feat(http-plugin): add options to disable new spans if no parent #931 #948

merged 7 commits into from
May 6, 2020

Conversation

vmarchaud
Copy link
Member

No description provided.

@vmarchaud vmarchaud self-assigned this Apr 8, 2020
@vmarchaud vmarchaud added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 8, 2020
@vmarchaud vmarchaud force-pushed the http-option-require-parent branch 3 times, most recently from 6efb8b0 to 8de7045 Compare April 12, 2020 12:00
@vmarchaud
Copy link
Member Author

As #930, lint is failing because of GTS versions

@codecov-io
Copy link

codecov-io commented Apr 12, 2020 •
edited
Loading

Codecov Report

Merging #948 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
+ Coverage   95.01%   95.06%   +0.05%     
==========================================
  Files         212      212              
  Lines        8827     8898      +71     
  Branches      797      801       +4     
==========================================
+ Hits         8387     8459      +72     
+ Misses        440      439       -1
Impacted Files Coverage Δ
packages/opentelemetry-plugin-http/src/http.ts 97.70% <100.00%> (+0.12%) ⬆️
...y-plugin-http/test/functionals/http-enable.test.ts 96.88% <100.00%> (+0.59%) ⬆️
...lemetry-plugin-http/test/utils/DummyPropagation.ts 100.00% <0.00%> (+6.66%) ⬆️

@vmarchaud vmarchaud force-pushed the http-option-require-parent branch from 8de7045 to 82ba62e Compare April 15, 2020 08:46
OlivierAlbertini
OlivierAlbertini reviewed Apr 16, 2020
View reviewed changes
Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look good

@vmarchaud vmarchaud force-pushed the http-option-require-parent branch from 82ba62e to ef451f6 Compare April 18, 2020 12:09
@dyladan
Copy link
Member

dyladan commented Apr 18, 2020

@open-telemetry/javascript-approvers we need more reviews on this

@vmarchaud vmarchaud force-pushed the http-option-require-parent branch from ef451f6 to be6e3cd Compare April 25, 2020 16:00
@vmarchaud vmarchaud requested review from obecny and dyladan April 25, 2020 16:01
@vmarchaud vmarchaud force-pushed the http-option-require-parent branch 2 times, most recently from 77e9ee9 to e35ff55 Compare April 25, 2020 16:03
dyladan
dyladan approved these changes Apr 27, 2020
View reviewed changes
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks

@dyladan dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Apr 29, 2020
@vmarchaud vmarchaud force-pushed the http-option-require-parent branch from 42e9c10 to cb1e933 Compare May 2, 2020 13:17
obecny
obecny approved these changes May 6, 2020
View reviewed changes
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, one comment

packages/opentelemetry-plugin-http/src/http.ts Outdated Show resolved Hide resolved
@vmarchaud vmarchaud force-pushed the http-option-require-parent branch from 6eed95e to 4cd48d1 Compare May 6, 2020 15:51
@dyladan dyladan merged commit e5a31d7 into open-telemetry:master May 6, 2020
@vmarchaud vmarchaud mentioned this pull request May 10, 2020
Closed
@dyladan
Copy link
Member

dyladan commented Aug 4, 2020

@vmarchaud can you help me understand why you decided to create a non recording span rather than just returning the NOOP span provided by the API?

You've introduced a hard dependency on our core/sdk, which means a third party implementation of an SDK will not be able to work with this plugin.

@vmarchaud
Copy link
Member Author

@dyladan Well if i remember correctly using a NoopSpan wasn't propagating the context, but if i'm mistaken i see no reason to not use a NoopSpan instead

@dyladan
Copy link
Member

dyladan commented Aug 4, 2020

I would like to remove the dependency on core for all plugins if possible. In the current state, plugins are only compatible with the default sdk, but won't be interoperable with third party sdks. The http plugin is one of the only ones that depends on something other than BasePlugin in core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OlivierAlbertini OlivierAlbertini OlivierAlbertini approved these changes

@dyladan dyladan dyladan approved these changes

@mayurkale22 mayurkale22 mayurkale22 approved these changes

@obecny obecny obecny approved these changes

@bg451 bg451 Awaiting requested review from bg451

@markwolff markwolff Awaiting requested review from markwolff

@rochdev rochdev Awaiting requested review from rochdev

Assignees

@vmarchaud vmarchaud

Labels
Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vmarchaud @codecov-io @dyladan @obecny @OlivierAlbertini @mayurkale22